-
Notifications
You must be signed in to change notification settings - Fork 20
Update .lgd-row classes to use CSS grid #778
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Very happy about this, but there are still some weird alignment issues which I've been meaning to talk to you about. The culprit is Are you also thinking of rolling this out to the multi-column layouts (in subsites)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make a good new default (I typically do more or less this on new projects).
Yes, definitely. The basic issue is that it's used everywhere--on page-level layouts, regions, and components. It means that sometimes It's hard to think about how to roll out fixes for it to existing sites without breaking all the overrides we incorporated when building them in the first place. On new sites, it's possible to use some slightly hairy css to keep a lid on the problem, something like the code below, but it's design-dependent, so the selectors in the Site-specific .padding-horizontal rationalization/**
* LGD's .padding-horizontal can be unhelpful, especially when nested.
*
* This file selectively overrides .padding-horizontal by region and content
* type, essentially by removing padding-inline from the element with the class
* and adding it to the parent element, making exceptions for the children of
* certain grid-cells.
*
* The main selector does the following:
*
* - :where():has(): targets specific regions and specific 'full' content types
* that have a child element matching .padding-horizontal,
* - .lgd-row:not() &: (content types only) targets elements from the
* :where():has() that are NOT children of specific grid-cells, and ADDS
* padding-inline to them,
* - & > .padding-horizontal: targets child elements from the :where:has() that
* have the .padding-horizontal class and REMOVES padding-inline from them.
*/
/* Regions are relatively simple */
:where(
/* Add regions where .padding-horizontal isn't helping. */
.region-breadcrumb,
.region-content-bottom,
.region-messages,
.region-tabs,
.region-content-top,
):has(> .padding-horizontal) {
padding-inline: var(--spacing-mobile-gutter);
& > .padding-horizontal {
padding-inline: 0;
}
}
/* Content types are slightly trickier */
:where(
/* Add node content elements here where .padding-horizontal isn't helping. */
.lgd-maintenance,
.lgd-status,
.localgov-alert-banner,
.localgov-services-page.node--view-mode-full,
.localgov-subsites-overview.node--view-mode-full,
.node--type-localgov-guides-overview.node--view-mode-full,
.node--type-localgov-guides-page.node--view-mode-full,
.node--type-localgov-news-article.node--view-mode-full,
.localgov-subsites-overview.node--view-mode-full,
.localgov-step-by-step-overview.node--view-mode-full,
.localgov-step-by-step-page.node--view-mode-full,
.lgd-page-section--full-width-contained-content,
):has(> .padding-horizontal) {
/* Add padding-inline to regions/node-content on non-sidebar pages. */
.main:not(:has(.sidebar)) & {
padding-inline: var(--spacing-mobile-gutter);
}
& > .padding-horizontal {
padding-inline: 0;
}
}
/**
* Children of .main--this doesn't include regions--get styled to match.
*
* NOTE: some components handle this in their own stylesheets as they require
* further customization.
*/
.main > .lgd-region--content:first-child,
.main > .lgd-region--content-top + .lgd-region--content,
.main > .lgd-container {
margin-block-start: var(--vertical-rhythm-spacing);
}
.main > .lgd-container {
box-sizing: content-box;
margin-inline: auto;
max-width: var(--width-extra-large);
padding-inline: var(--spacing-mobile-gutter);
}
/* Remove padding-inline from immediate children of grid cells generally. */
.lgd-row
> :where(
.lgd-row__one-quarter,
.lgd-row__three-quarters,
.lgd-row__one-third,
.lgd-row__two-thirds,
.lgd-row__one-half,
)
> .padding-horizontal {
padding-inline: 0;
}
@media screen and (min-width: 64rem) {
.main > .lgd-region--content:first-child,
.main > .lgd-region--content-top + .lgd-region--content,
.main > .lgd-container {
margin-block-start: calc(2 * var(--vertical-rhythm-spacing));
}
} On a recent non-LGD site, I introduced a new class
As long as regions, components used this basic structure, everything worked perfectly well, and it was possible to mix and match the <div class="section section--contained">
<div class="section__inner">
<!-- Content -->
</div>
</div> It would work for LGD too, though without a major version increase, all we could really do would be to ship a stylesheet that was scoped by a non-default style, or not added to the theme's libraries file--we'd still have to remove/override the existing |
I'm definitely not against a major version increase. |
Discussing in Merge Tuesday, we're happy with this as a minor version release, not major. |
This reverts commit ffa1c45.
Closes #666
What does this change?
Rewrites our
.lgd-row
grid system to use CSS Grid instead of flexbox. This means we will no longer have the annoying issue with the small margins on the left/right of grids that mean things don't line up as nicely as we'd like them to without extra CSS overrides.How to test
Click around the website and make sure the grid is working, but more importantly marvel at how all the items line up correctly.
How can we measure success?
Waaaaay few overrides for our layout in custom themes.
Images